Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

IDE changes for "Unsigned Right Shift" feature, plus tests. #60689

Merged
merged 6 commits into from
Apr 13, 2022
Merged

IDE changes for "Unsigned Right Shift" feature, plus tests. #60689

merged 6 commits into from
Apr 13, 2022

Conversation

AlekseyTs
Copy link
Contributor

@AlekseyTs AlekseyTs commented Apr 11, 2022 •
edited by jcouv
Loading

Test plan #60433

@AlekseyTs AlekseyTs requested review from jasonmalinowski, 333fred, jcouv and a team April 11, 2022 22:10
@AlekseyTs AlekseyTs requested review from a team as code owners April 11, 2022 22:10
jcouv
jcouv reviewed Apr 11, 2022
View reviewed changes
src/EditorFeatures/CSharpTest/Formatting/FormattingEngineTests.cs Outdated
var code = @"$$
class C
{
public static C operator >>> ( C x, C y){
Copy link
Member

@jcouv jcouv Apr 11, 2022 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: consider starting with no space or two spaces between operator and >>>, to show that part got formatted #Resolved

jcouv
jcouv reviewed Apr 11, 2022
View reviewed changes
src/EditorFeatures/CSharpTest/QuickInfo/SemanticQuickInfoSourceTests.cs
@"int x;

x = x >>>$$ x;",
MainDescription("int int.operator >>>(int left, int right)"));
Copy link
Member

@jcouv jcouv Apr 11, 2022 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a similar QuickInfo scenario for >>>=? (I'm not sure what's the expected behavior) #Resolved

jcouv
jcouv reviewed Apr 11, 2022
View reviewed changes
src/VisualStudio/CSharp/Test/F1Help/F1HelpTests.cs
@@ -328,6 +328,7 @@ class C<T>
[InlineData("<=")]
[InlineData("<<")]
[InlineData(">>")]
[InlineData(">>>")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tagging @BillWagner as we'll want landing pages for F1/Help on >>> and >>>=

jcouv
jcouv reviewed Apr 11, 2022
View reviewed changes
src/Workspaces/CSharp/Portable/CodeGeneration/CSharpSyntaxGenerator.cs
@@ -292,6 +292,7 @@ private static SyntaxKind GetTokenKind(OperatorKind kind)
OperatorKind.Multiply => SyntaxKind.AsteriskToken,
OperatorKind.OnesComplement => SyntaxKind.TildeToken,
OperatorKind.RightShift => SyntaxKind.GreaterThanGreaterThanToken,
OperatorKind.UnsignedRightShift => SyntaxKind.GreaterThanGreaterThanGreaterThanToken,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another scenario that may be worth validating is MetadataAsSource. Hopefully, it's using the same syntax generator and is fine.

jcouv
jcouv approved these changes Apr 11, 2022
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thanks (iteration 2)

@jcouv jcouv self-assigned this Apr 11, 2022
@AlekseyTs
Copy link
Contributor Author

@jasonmalinowski, @333fred Please review

@AlekseyTs
Copy link
Contributor Author

@jasonmalinowski Please review

jasonmalinowski
jasonmalinowski approved these changes Apr 12, 2022
View reviewed changes
Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDE part looks good. Only comments are some potential places for additional tests, but no reason to hold up this PR for that.

src/Compilers/CSharp/Portable/Syntax/SyntaxKindFacts.cs
@@ -143,7 +143,7 @@ public static IEnumerable<SyntaxKind> GetPreprocessorKeywordKinds()

public static bool IsPunctuation(SyntaxKind kind)
{
return kind >= SyntaxKind.TildeToken && kind <= SyntaxKind.QuestionQuestionEqualsToken;
return kind >= SyntaxKind.TildeToken && kind <= SyntaxKind.GreaterThanGreaterThanGreaterThanEqualsToken;
Copy link
Member

@jasonmalinowski jasonmalinowski Apr 12, 2022 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also including !! that wasn't included before; that seems reasonable to me but we think that was just an oversight before? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also including !! that wasn't included before; that seems reasonable to me but we think that was just an oversight before?

I think it was. CC @RikkiGibson

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it was an oversight. It looks like this could change behavior of classification of !!, but I must not have added any tests for !! to src/EditorFeatures/CSharpTest/Classification/SyntacticClassifierTests.cs.

src/Compilers/CSharp/Portable/Syntax/SyntaxKindFacts.cs
@@ -164,7 +164,7 @@ private static bool IsDebuggerSpecialPunctuation(SyntaxKind kind)

public static IEnumerable<SyntaxKind> GetPunctuationKinds()
{
for (int i = (int)SyntaxKind.TildeToken; i <= (int)SyntaxKind.PercentEqualsToken; i++)
for (int i = (int)SyntaxKind.TildeToken; i <= (int)SyntaxKind.GreaterThanGreaterThanGreaterThanEqualsToken; i++)
Copy link
Member

@jasonmalinowski jasonmalinowski Apr 12, 2022 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be a test that each kind returned from GetPunctuationKinds returns true for IsPunctuationKind? Since it seems there were other existing bugs here too. #Resolved

...esAndExtensions/Workspace/CSharp/LanguageServices/CSharpTypeInferenceService.TypeInferrer.cs

if (onRightOfToken)
{
// x << Goo(), x >> Goo(), x <<= Goo(), x >>= Goo()
// x << Goo(), x >> Goo(), x >>> Goo(), x <<= Goo(), x >>= Goo(), x >>>= Goo()
Copy link
Member

@jasonmalinowski jasonmalinowski Apr 12, 2022 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

roslyn/src/EditorFeatures/CSharpTest/TypeInferrer/TypeInferrerTests.cs

Lines 194 to 199 in b0cd71f

[Theory, CombinatorialData, Trait(Traits.Feature, Traits.Features.TypeInferenceService)]
public async Task TestBinaryOperator2(TestMode mode)
{
await TestInMethodAsync(
@"var q = x >> [|Goo()|];", "global::System.Int32", mode);
}
would be where to add a test for this. #Resolved

@runfoapp runfoapp bot mentioned this pull request Apr 13, 2022
Closed
@AlekseyTs AlekseyTs enabled auto-merge (squash) April 13, 2022 21:51
@AlekseyTs AlekseyTs merged commit 55b8fae into dotnet:features/UnsignedRightShift Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RikkiGibson RikkiGibson RikkiGibson left review comments

@jcouv jcouv jcouv approved these changes

@333fred 333fred 333fred approved these changes

@jasonmalinowski jasonmalinowski jasonmalinowski approved these changes

Assignees

@jcouv jcouv

Labels
Area-Compilers Feature - UnsignedRightShift
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AlekseyTs @jasonmalinowski @333fred @RikkiGibson @jcouv